-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(search): hide feedback from issues search by default #76984
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #76984 +/- ##
==========================================
+ Coverage 78.16% 78.24% +0.08%
==========================================
Files 6910 6899 -11
Lines 307185 306905 -280
Branches 50342 50265 -77
==========================================
+ Hits 240097 240143 +46
+ Misses 60720 60402 -318
+ Partials 6368 6360 -8 |
src/sentry/search/snuba/executors.py
Outdated
else: | ||
all_group_type_objs: list[GroupType] = [ | ||
gt_registry.get_by_type_id(id) | ||
for id in gt_registry.get_all_group_type_ids() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do this like this, on line 1828
if not is_errors:
visible_group_types = group_types_from(search_filters)
if visible_group_types is None:
# When no group types are specified, default to all default group types
visible_group_types = [gt for gt in all_group_type_objs if not gt.in_default_search]
# remove `raw_group_types is not None` check, since it'll never be none now, just potentially empty
# Then just run the same logic we were running before
This keeps the logic shared which should reduce the chance of bugs. Since in your new branch you missed grouptype.registry.get_visible
, and so we'd have started showing unreleased issue types to all users.
Reading this code, this might actually be a bug already... if no issue types are specified, we're not filtering to visible issue types only. So doing this will fix the bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, this line should be like this right?
[gt for gt in all_group_type_objs if not gt.in_default_search]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the visible filter seems to happen in the if not None case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the logic to the base case of group_types_from
. For some reason this causes test_snuba_perf_issue to fail, not sure why, it worked once locally so could be flakey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the test, it's because the GroupType created by storing a transaction, 2001 ProfileFileIOGroupType, is not registered.
Not sure how GroupTypes are registered in prod, but this change makes it so by default, issues with unregistered GroupTypes are never listed (before, we would just skip the filter).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change makes it so by default, issues with unregistered GroupTypes are never listed (before, we would just skip the filter).
Do we expect all group types to be registered in prod? Is it ok to manually register the 2001 type to pass this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not totally sure what you mean by it not being registered? It might just be because the group type hasn't been marked as visible?
You can do that like
sentry/tests/snuba/search/test_backend.py
Line 3583 in 9d25078
with self.feature([ProfileFileIOGroupType.build_visible_feature_name()]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh got it, that was it, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for fixing this!
Follow up to #59505 to filter feedbacks from the default search when querying Postgres. As before, filtering on issue.category:feedback or a negation (ex !issue.category:error) will still return feedback issues. Fixes #60697 Fixes getsentry/team-replay#470
Follow up to #59505 to filter feedbacks from the default search when querying Postgres. As before, filtering on issue.category:feedback or a negation (ex !issue.category:error) will still return feedback issues.
Fixes #60697
Fixes https://github.com/getsentry/team-replay/issues/470